Prevent secret leaks from environment table wrapping - #3561
Conversation
There was a problem hiding this comment.
Review: Prevent secret leaks from environment table wrapping
The fix correctly addresses the root cause from #3548: NoWrap + Ellipsis() on the value column keeps each env value on one physical console row (so runner-side ::add-mask:: can still match), MakeSingleLine neutralizes embedded CR/LF as a belt-and-suspenders measure, and the switch from Markup.Escape(...) strings to Text cells is a nice side benefit — Text never interprets markup, so it's safer against control-sequence injection than the old escaped-string approach. Tests cover the wrap/mask/CRLF cases well, including a dedicated narrow-width render check.
One architectural point worth considering, not a blocker:
Sensitive-name detection duplicates an existing configurable pattern instead of reusing it. HttpRequestFormatter/HttpResponseFormatter already solve the "which names are secret-ish" problem via HttpLoggingOptions.SensitiveHeaderNames — a public, IOptions-configurable, user-overridable list with a documented default. This PR introduces a second, parallel mechanism: SensitiveEnvironmentVariableNameParts is a private static readonly string[] baked into PipelineInitializer, with no way for a consuming pipeline to extend or override it.
Why that matters here specifically: the substring match (name.Contains("PWD", ...), .Contains("KEY", ...)) is intentionally broad per the issue's spec, but it also means standard, non-secret shell variables like PWD/OLDPWD are now always masked, and any consumer whose own naming convention happens to contain one of these substrings (e.g. a TURNKEY_ENDPOINT or PRIMARY_KEY_COLUMN env var) loses visibility into that value in the startup table with no opt-out. The HttpLoggingOptions precedent exists precisely to let consumers tune this kind of heuristic per-project.
Suggestion: lift SensitiveEnvironmentVariableNameParts into a small options record (or extend SecretMaskingOptions) following the HttpLoggingOptions.SensitiveHeaderNames shape — public static Default list, IOptions<T>-injected, user-overridable via PipelineBuilder.Services. That keeps the two "mask by name heuristic" implementations in the codebase consistent, and gives consumers an escape hatch for false positives without needing another PR/fork. Not required for this fix to be correct or safe to ship, but worth a follow-up given PWD will now be masked in effectively every pipeline run.
Everything else looks solid — validation section in the PR description matches what I see in the diff, and I didn't find any correctness issues with the masking/truncation logic itself.
There was a problem hiding this comment.
Code review
No new changes since the previous review (the only commit added since 00ac5f7 is a merge of main, with no functional diff to the touched files) — so this is a verification pass rather than a fresh review.
Independently verified locally against the current HEAD (a7ccb9b):
dotnet build ModularPipelines.sln -c Release: 0 errors, 0 warningsPipelineInitializerTestsvia TUnit--treenode-filter: 10/10 passed
The masking/truncation logic in CreateEnvironmentVariablesTable holds up: sensitive-name detection short-circuits before obfuscation runs, NoWrap + .Ellipsis() keeps each value on one physical console row, and MakeSingleLine escapes embedded CR/LF as a second line of defense. Switching from Markup.Escape(...) strings to Text cells is a good safety improvement too. No correctness issues found.
Outstanding architectural point from the prior review — still unaddressed, still not a blocker:
SensitiveEnvironmentVariableNameParts (PipelineInitializer.cs#L24-L32) is a hardcoded private static readonly string[] with no way for a consuming pipeline to extend or override it. This duplicates a pattern the codebase already solves correctly elsewhere:
SecretMaskingOptions(src/ModularPipelines/Options/SecretMaskingOptions.cs) already exists as a DI-configurable options record for exactly this kind of "how do we detect/mask secrets" behavior — consumers configure it viabuilder.Services.Configure<SecretMaskingOptions>(...).HttpLoggingOptions.SensitiveHeaderNamesfollows the same shape (IReadOnlyList<string>+ staticDefault) for the equivalent "which names are secret-ish" problem on the HTTP logging side.
Concretely, the substring match here is intentionally broad (Contains("KEY"), Contains("PWD"), etc.), so it now always masks PWD/OLDPWD and any variable whose name happens to contain one of these substrings (e.g. TURNKEY_ENDPOINT, PRIMARY_KEY_COLUMN). Every consumer of this framework loses visibility into those values in the startup table, with no opt-out short of a fork or a follow-up PR to this repo.
Suggested follow-up: fold the sensitive name-part list into SecretMaskingOptions and inject it into PipelineInitializer the same way ISecretObfuscator is already injected. That keeps the two "mask by name heuristic" mechanisms in the codebase consistent and gives consumers a real escape hatch for false positives.
Not required for this fix to be correct or safe to ship — flagging again since it's still open, for the author/maintainer to decide whether to track as a follow-up issue.
There was a problem hiding this comment.
Code review
Reviewed the secret-masking / table-rendering changes in PipelineInitializer.CreateEnvironmentVariablesTable. The masking logic itself (name-heuristic short-circuit before obfuscation, CR/LF normalization applied after obfuscation, ordinal-ignore-case matching, Text instead of Markup.Escape to avoid re-interpretation) is sound and the new tests cover the intended behaviors well. One high-signal correctness bug found in the layout change — see inline comment.
No CLAUDE.md compliance issues found (only the root CLAUDE.md applies to these files, and the changes don't touch build/CI/module/codegen conventions it governs).
df0a3b6 to
2587cd8
Compare
|
Addressed the unresolved narrow-terminal review finding in
TDD: the focused test failed before the production change because the name rendered one character per line; it now passes. Validation:
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2587cd8 to
6ee031e
Compare
|
Rebased onto current @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ee031ef63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code reviewNo issues found. Checked for bugs, CLAUDE.md compliance, and correctness of the secret-masking logic. What I checked:
No CLAUDE.md violations, no compile/logic issues, no gaps in the masking heuristic that would defeat the PR's stated purpose. |
6ee031e to
9962bc3
Compare
|
Addressed the P2 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9962bc32f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code reviewNo issues found. This is a follow-up review of the changes since the last Claude review — specifically commit What changed since the last review:
Validation performed:
Nothing further to flag — the PWD/OLDPWD fix is correct, narrowly scoped, and tested. |
9962bc3 to
5409ad9
Compare
|
Addressed authentication-bearing environment names in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5409ad9e42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code reviewReviewed the latest commit, What changed: Correctness: the exemption check runs before the substring Architectural point (repeating from earlier review rounds, still unaddressed — not a blocker): this commit is a good illustration of why the earlier suggestion still stands. Each round has added one more hardcoded substring to The codebase already has the right shape for this in two places:
Since this PR keeps discovering new substrings/exemptions through review iteration, that's a reasonable signal the heuristic has stabilized enough to lift into an options record (sensitive name-parts + exemption list) injected into No bugs or CLAUDE.md violations found in this commit's diff. Tests are well-targeted for the new behavior. |
|
Addressed the latest P1 review by masking documented |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf8a51b782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code reviewReviewed the latest commit, What changed: adds a new Correctness: the new Architectural point (repeating from earlier review rounds — still unaddressed, still not a blocker, but worth escalating): this commit adds a third hardcoded, private list to this same mechanism —
Folding Not required for this fix to be correct or safe to ship. Given this is now the third round adding to the same hardcoded heuristic, I'd suggest tracking the options-record refactor as a concrete follow-up issue rather than continuing to patch the array in-place. |
|
Expanded masking to standard, .NET, and Azure connection-string environment variable conventions with regression coverage. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c0533b7a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6d022198a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f02d0c7d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98098861c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ea6007fbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27fe1dbce2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 894d12f64b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f144d066
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44dfe526f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "KEY", | ||
| "PWD", | ||
| "CREDENTIAL", | ||
| "AUTH", |
There was a problem hiding this comment.
Preserve AZURE_STORAGE_AUTH_MODE diagnostics
When an Azure storage pipeline sets the standard AZURE_STORAGE_AUTH_MODE variable to key or login, this undelimited AUTH match masks that non-secret mode. Fresh evidence beyond the earlier authentication-name exemptions is the repository's own definition of these allowed values in src/ModularPipelines.Azure/Options/AzStorageContainerMetadataShowOptions.Generated.cs:24; because the exact non-sensitive allowlist omits this name, trace output can no longer show which authentication mode was selected. Exempt this exact setting or make the authentication match boundary-aware.
Useful? React with 👍 / 👎.
Summary
Textcells so long or multiline values never become multiple physical log linesSecretMaskingOptions.MaskValuefor name-based masking, including the safe default fallback for blank configurationPWD,OLDPWD, andSSH_AUTH_SOCKdiagnosticsValidation
PipelineInitializerTests: 25 passed, covering narrow-console truncation, CR/LF normalization, sensitive-name conventions, exemptions, and configured mask behaviorModularPipelines.slnRelease build: 0 errors; 227 existing warningsRotation/expiry of any credentials exposed by historical logs remains an external owner/security action.
Closes #3548
Follow-up review fix
PipelineInitializerTests(26/26 passing).Render-width follow-up
CONFIGvalue at width 40 plus the exact safe-fit boundary.PipelineInitializerTests: 28/28 passing; scoped formatter clean.Header-width follow-up
Nameheader width when calculating safe raw-value capacity, preventing Spectre padding around one-character variable names from exposing truncated raw values.PipelineInitializerTests(29/29 passing); scoped whitespace formatter clean.Partial-obfuscation follow-up
PipelineInitializerTests: 30/30 passed; scoped whitespace formatter clean.